Conversation
Integration proxy responses were missing the x-synthetic-id header because handle_proxy dispatched to integrations without adding it. This caused identity tracking to break for first-party re-hosted integrations like Permutive's secure-signal endpoint. Centralizing the header logic in handle_proxy ensures all current and future integrations automatically include the synthetic ID, rather than requiring each integration to implement it manually. Fixes #205
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Good start but should use funcitons in cookies.rs
Please don't make the feature branch have this long name. Please stick to feature/short-name
Replaces inline cookie parsing with handle_request_cookies() and parse_cookies_to_jar() from cookies.rs for consistency.
Had deleted the comment accidentally but was needed already
There was a problem hiding this comment.
🔧 We need a more thorough fix:
publisher.rs:201-211 uses manual string splitting to check for an existing synthetic cookie:
let has_synthetic_cookie = req
.get_header(header::COOKIE)
.and_then(|h| h.to_str().ok())
.map(|cookies| {
cookies.split(';').any(|cookie| {
cookie.trim_start().starts_with(&format!("{}=", COOKIE_SYNTHETIC_ID))
})
})
.unwrap_or(false);
While registry.rs:622-626 uses handle_request_cookies with CookieJar:
let has_synthetic_cookie = handle_request_cookies(&req)
.ok()
.flatten()
.and_then(|jar| jar.get(COOKIE_SYNTHETIC_ID).map(|_| true))
.unwrap_or(false);
These should use the same approach. The CookieJar version is more robust (proper cookie parsing).
Since we already have cookie utilities in cookies.rs, suggest adding a set_synthetic_cookie(settings: &Settings, response: &mut Response, synthetic_id: &str) helper there that handles the append_header call. That way both publisher.rs and registry.rs call the same function, and the append_header fix and cookie creation logic live in one place.
We don't need has_synthetic_cookie check we was primarily for logging we should always set COOKIE_SYNTHETIC_ID
…te Testlight to read them, while also adding a test for synthetic cookie setting
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
🔧 We inadvertently now passing HEADER_X_SYNTHETIC_ID header to downstream integrations. We should not because synthetic header should stay within TS.
For example: permutive.rs :502
Please audit al integrations not to pass HEADER_X_SYNTHETIC_ID header
…vices - Fix missing `x-synthetic-id` on `/integration` responses by setting it in `registry.rs` - Introduce `INTERNAL_HEADERS` constant list for sensitive internal headers - Add `http_util::copy_custom_headers` utility to filter internal headers - Update `permutive.rs` and `lockr.rs` to use safe header copying, preventing leaks - Add unit tests verifying header filtering logic
Thanks @aram356 for the catch on the header leak! I've addressed it by:
This ensures Added unit tests to |
- Add x-forwarded-for to INTERNAL_HEADERS to prevent IP leak to downstream services via copy_custom_headers - Fix stale x-psid-ts doc comment in synthetic.rs - Use proper import for get_synthetic_id in testlight.rs and improve error message for missing synthetic ID - Fix typo in http_util.rs test
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
It's looking pretty good to me now
aram356
changed the title
Manually verified locally via
|
prk-Jr
dismissed
ChristianPavilonis’s stale review
Stale review. Has been resolved.
3 participants
Integration proxy responses were missing the
x-synthetic-idheader becausehandle_proxydispatched to integrations without adding it. This caused identity tracking to break for first-party re-hosted integrations like Permutive's secure-signal endpoint.Centralizing the header logic in
handle_proxyensures all current and future integrations automatically include the synthetic ID, rather than requiring each integration to implement it manually.Test Plan
Manually verified locally via
fastly compute serveagainst autoblog production config:GET /integrations/permutive/secure-signals→x-synthetic-idheader andSet-Cookie: synthetic_id=...presentGET /integrations/lockr/api→ sameGET /(publisher proxy) → samex-synthetic-idis not forwarded to downstream third-party origins (filtered byINTERNAL_HEADERSviacopy_custom_headers)Fixes #205